Refactored docs for std::fs::set_permissions_nofollow + fix BSD-based systems to use fchmodat with AT_SYMLINK_NOFOLLOW flag - #160170

These aren't super necessary but 🤷🏻 doesn't matter too much

View changes since the review

Yeah this is grammatically still odd -- the first two bullets are not actually sentences, but the last one is.

That flag is enabled where? Should this say "SetFileInformationByHandle with the flag ..." or would that not be right?

View changes since the review

The internal code for Windows' set_permissions_nofollow does the following:

pub fn set_perm_nofollow(p: &WCStr, perm: FilePermissions) -> io::Result<()> {
    let mut opts = OpenOptions::new();
    opts.access_mode(c::FILE_WRITE_ATTRIBUTES);
    // `FILE_FLAG_OPEN_REPARSE_POINT` for no_follow behavior
    opts.custom_flags(c::FILE_FLAG_BACKUP_SEMANTICS | c::FILE_FLAG_OPEN_REPARSE_POINT);
    let file = File::open_native(p, &opts)?;
    file.set_permissions(perm)
}

It opens the file first with FILE_FLAG_BACKUP_SEMANTICS and FILE_FLAG_OPEN_REPARSE_POINT and then it sets the permissions on the reparse point itself using SetFileInformationByHandle. Enabled is probably a poor choice of word here; at the time, I wasn't too sure how to discuss the opening a file behavior on Windows because it seems like OpenOptions::open doesn't go in depth about platform-specific behavior and the open_native call seem to be doing a lot here:

I would say something like

/// This function currently corresponds to the following underlying operations:
/// * WASI: `open` with `O_NOFOLLOW` followed by `fchmod`.
/// * Unix: `fchmodat` with `AT_SYMLINK_NOFOLLOW`.
/// * Windows: `CreateFileW` with `FILE_FLAG_OPEN_REPARSE_POINT` followed
///   by `SetFileInformationByHandle`.

I took a look at Unix implementation of set_permissions_nofollow again, and I wanted to correct something I said about MacOS/BSD-based systems. Currently, they call fchmodat with no flag provided (0), and I realized that's a mistake I made since I forgot to include them underneath the fchmodat with AT_SYMLINK_NOFOLLOW.

I think the reason why I did that was because CI mentioned compiler errors on libc::AT_SYMLINK_NOFOLLOW not existing on certain platforms (though this was on dist-various-1 so wasn't sure what that tested), so I gated it to linux, but forgot to update it to include MacOS/other BSD-based systems that support AT_SYMLINK_NOFOLLOW.

I have to fix this and include the BSD-based systems in that block. I also verified the behavior on my MacBook and it should definitely change the permission bits, which means the test case that sets a symlink with readonly permission should pass.

Android and Linux share the same kernel, so I highly doubt they will behave different here?

View changes since the review

I'll take it off from this block and we can put up a try job to confirm

            // On these systems, the symlink itself is now marked readonly.

This is meant to replace the entire comment, github is just incapable of making that a suggestion...

View changes since the review

What is going on here? The not part at the end makes no sense at all...?

View changes since the review

We promise that we will not follow the symlink. So why is it correct to not set the flag?

View changes since the review

To be honest with you, I wasn't sure what Unix platforms didn't support AT_SYMLINK_NOFOLLOW/O_NOFOLLOW because they didn't support symlinks.

For example, I think in my previous PR someone was commenting that ESPIDF/Horizon OS do not have support O_NOFOLLOW or symlinks, so I figured that their behavior should match with just regularly setting permissions on the file (fchmodat with no flag set). I had the not condition from earlier because I wasn't exhaustively sure which Unix-based platform has no symlinks.

I could try separating the not portion of the second branch into its own branch using fchmodat with no flag set and then have the _ branch do the same thing that lolbinarycat did originally:

use crate::fs::OpenOptions;
use crate::os::unix::fs::OpenOptionsExt;

OpenOptions::new().custom_flags(libc::O_NOFOLLOW).open(path)?.set_permissions(perm)

^This does return a different error message than Unsupported on symlinks though.

I think in my previous PR someone was commenting that ESPIDF/Horizon OS do not have support O_NOFOLLOW or symlinks, so I figured that their behavior should match with just regularly setting permissions on the file (fchmodat with no flag set)

If we are sure this is correct (ping the target maintainers) then we can skip the flag on those targets.

But your code skips the flag on all unknown targets and that doesn't seem good.

OpenOptions::new().custom_flags(libc::O_NOFOLLOW).open(path)?.set_permissions(perm) seems like a good fallback impl.

Regarding ESPIDF/Horizon OS, I found the PR that made the change after lolbinarycat's implementation:
#145746

It's confirmed in this issue that ESPIDF has no libc::O_NOFOLLOW because its supported filesystems don't use symbolic links

The maintainer of the ESP-IDF target here: yes, I confirm ESP-IDF (and likely Horizon) do not support neither fchmodat, nor AT_SYMLINK_NOFOLLOW, nor O_NOFOLLOW so they should fall back to the old code before #158168. The latest code surface of this PR seems to fix that.

To be clear, this only makes sense because they don't support symlinks at all. We have to guarantee that these operations never follow symlinks.

Do we know any platform where it does nothing at all?

View changes since the review

I don't have an exhaustive list. All I read from this MacOS post is that it's possible some systems can't change symbolic link permissions at all, which I interpreted to mean two different things: that fchmodat has no effects on symlinks or it throws an error. I did see this issue here that mentions that certain kernel silently ignored AT_SYMLINK_NOFOLLOW, which change permissions on the target, but that does something instead of no effect and it's an incorrect implementation.

Should I assume that AT_SYMLINK_NOFOLLOW should succeed in setting the permission bits/throw an error if it's not supported on symlinks? I'm unsure if no effects on symlinks and not throwing an error is valid behavior.

Actually, I'll remove the no effects behavior note. I don't think of it being a valid behavior for this function, and since I can't find any results that tell me what platforms does nothing for fchmodat with AT_SYMLINK_NOFOLLOW

I don't understand this comment. Which "other distributions"? Do different Linuxes behave different here, despite all using the same kernel? Which system returns "unsupported" to indicate a symlink?

View changes since the review

See Rachel's comment earlier. Ubuntu 20.04 throws ENOTSUP when using fchmodat with AT_SYMLINK_NOFOLLOW regardless if the file is a symlink or not. For that platform specifically, we have to use open with O_NOFOLLOW flag and then fchmod. open with O_NOFOLLOW would throw an ELOOP error in this scenario.

Maybe consistency was a bad choice of word here, but I thought it was better to keep the error message to what fchmodat would fail with (ENOTSUPP/Unsupported) instead of using ELOOP here (other reason why is because FilesystemLoop error is unstable and Unsupported is stable, so maybe convenient for those to error handle this without needing nightly features).

Doesn't that just mean that Ubuntu 20 has an old kernel that does not yet support fchmodat? We then trigger the fallback behavior anyway so the user never sees that. What kind of consistency are you trying to achieve here...?

fchmodat with AT_SYMLINK_NOFOLLOW in later versions of Ubuntu functions normally with setting permissions on regular files/directories and throws Unsupported on symlinks.

And the consistency thing is bad wording on my end. I might've even wrote that comment before getting rid of the cfg_select branches because we had other Unix/Linux platforms solely calling fchmodat and other platforms just calling fchmodat with fallback behavior to open and fchmod on Unsupported error. Now, I just have every platform that supports fchmodat call fchmodat and everything will fall to use open and fchmod.

I'll make changes to the comment in an amended commit later today.

fchmodat with AT_SYMLINK_NOFOLLOW in later versions of Ubuntu functions normally with setting permissions on regular files/directories and throws Unsupported on symlinks.

I assume not just later Ubuntu, but all recent Linux distros? Ubuntu uses a fairly standard kernel AFAIK?

The man page just says

     AT_SYMLINK_NOFOLLOW
            If path is a symbolic link, do not dereference it: instead operate on the link itself.

so it is a bit odd that we get "unsupported" at all...

I don't understand this comment. Which "other distributions"? Do different Linuxes behave different here, despite all using the same kernel? Which system returns "unsupported" to indicate a symlink?

View changes since the review

I wrote that comment, what I meant is that:

1. On Ubuntu 24.04 (the ideal path), fchmodat() with AT_SYMLINK_NOFOLLOW works on normal files, and returns ENOTSUP if the target is a symlink
2. On Ubuntu 20.04 (which we, at least, need support for), fchmodat() with AT_SYMLINK_NOFOLLOW always returns ENOTSUP
3. On both versions, openat() + fchmod() with AT_SYMLINK_NOFOLLOW works on normal files, and returns ELOOP if the target is a symlink

So the remapping is to make case (3) consistent with case (1), in terms of what error we return to the caller

As @asder8215 says, the code has been restructured since I wrote that, so the wording might need revising, but that's the idea.

View changes since the review